Skip to content

Stricter validation and error handling in GDS server & Server Method Call validation#3746

Draft
romanett wants to merge 13 commits into
masterfrom
romanett/GDSMethodCall
Draft

Stricter validation and error handling in GDS server & Server Method Call validation#3746
romanett wants to merge 13 commits into
masterfrom
romanett/GDSMethodCall

Conversation

@romanett
Copy link
Copy Markdown
Contributor

@romanett romanett commented May 8, 2026

Stricter validation and error handling in GDS server

  • Allow empty DatabaseStorePath for in-memory operation

  • Select database type dynamically in GdsNodeManagerFactory

  • Enforce required fields and argument validation in ApplicationsDatabaseBase

  • Prevent duplicate application URIs in LinqApplicationsDatabase

  • Propagate argument errors as OPC UA service exceptions in ApplicationsNodeManager

  • Remove obsolete attribute from ParsedNodeId

  • Update UAReferenceServer.ctt.xml test data with non-empty credentials

  • Improves robustness, security, error reporting, and test coverage

  • Introdcue MasterNodeManager call validation with permission checks and attribute caching

Types of changes

  • Bugfix (non-breaking change which fixes an issue)
  • Enhancement (non-breaking change which adds functionality)
  • Test enhancement (non-breaking change to increase test coverage)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected, requires version increase of Nuget packages)
  • Documentation Update (if none of the other choices apply)

Checklist

  • I have read the CONTRIBUTING doc.
  • I have signed the CLA.
  • I ran tests locally with my changes, all passed.
  • I fixed all failing tests in the CI pipelines.
  • I fixed all introduced issues with CodeQL and LGTM.
  • I have added tests that prove my fix is effective or that my feature works and increased code coverage.
  • I have added necessary documentation (if appropriate).
  • Any dependent changes have been merged and published in downstream modules.

@romanett
Stricter validation and error handling in GDS server
58ef601 
- Allow empty DatabaseStorePath for in-memory operation
- Select database type dynamically in GdsNodeManagerFactory
- Enforce required fields and argument validation in ApplicationsDatabaseBase
- Prevent duplicate application URIs in LinqApplicationsDatabase
- Propagate argument errors as OPC UA service exceptions in ApplicationsNodeManager
- Remove obsolete attribute from ParsedNodeId
- Refactor MasterNodeManager write/call validation to async with improved permission checks and attribute caching
- Update UAReferenceServer.ctt.xml test data with non-empty credentials
- Improves robustness, security, error reporting, and test coverage
@romanett
Copy link
Copy Markdown
Contributor Author

romanett commented May 8, 2026

ToDo: fix strange permissions only parameter in NodeManagers + MasterNodeManager
support #3692 in permisssions handling by exposing new NodeManager method that finds a method state from a CallRequest

@codecov
Copy link
Copy Markdown

codecov Bot commented May 8, 2026 •
edited
Loading

Codecov Report

❌ Patch coverage is 69.76744% with 52 lines in your changes missing coverage. Please review.
✅ Project coverage is 72.01%. Comparing base (96f8964) to head (9f7d8d2).

Files with missing lines Patch % Lines
...ies/Opc.Ua.Server/NodeManager/CustomNodeManager.cs 40.00% 13 Missing and 5 partials ⚠️
...pc.Ua.Server/NodeManager/AsyncCustomNodeManager.cs 71.05% 6 Missing and 5 partials ⚠️
...n/ApplicationsDatabase/ApplicationsDatabaseBase.cs 40.00% 8 Missing and 1 partial ⚠️
...pc.Ua.Gds.Server.Common/ApplicationsNodeManager.cs 53.33% 6 Missing and 1 partial ⚠️
...n/ApplicationsDatabase/LinqApplicationsDatabase.cs 90.47% 1 Missing and 1 partial ⚠️
...pc.Ua.Gds.Server.Common/Diagnostics/AuditEvents.cs 80.00% 2 Missing ⚠️
...er/NodeManager/Adapters/AsyncNodeManagerAdapter.cs 50.00% 1 Missing and 1 partial ⚠️
...ies/Opc.Ua.Server/NodeManager/MasterNodeManager.cs 97.36% 0 Missing and 1 partial ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #3746      +/-   ##
==========================================
- Coverage   72.14%   72.01%   -0.13%     
==========================================
  Files         597      598       +1     
  Lines      122192   122395     +203     
  Branches    20582    20618      +36     
==========================================
- Hits        88154    88147       -7     
- Misses      27997    28206     +209     
- Partials     6041     6042       +1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@romanett
Copy link
Copy Markdown
Contributor Author

romanett commented May 8, 2026

@copilot Please add Tests for ApplicationsDatabaseBase

@CLAassistant
Copy link
Copy Markdown

CLAassistant commented May 8, 2026

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

✅ romanett
❌ Copilot
You have signed the CLA already but the status is still pending? Let us recheck it.

Copy link
Copy Markdown
Contributor

Copilot AI commented May 8, 2026 •
edited
Loading

ToDo: fix strange permissions only parameter in NodeManagers + MasterNodeManager
support #3692 in permisssions handling by exposing new NodeManage...

Implemented this in c52718f (refined in 7d359cd): added new IMethodStateResolverNodeManager/IMethodStateResolverAsyncNodeManager APIs, implemented them in custom node managers/adapters, and updated MasterNodeManager call validation to resolve method state from CallMethodRequest via the new resolver path.

@copilot Please add Tests for ApplicationsDatabaseBase

Added coverage in c52718f (plus naming refinement in 7d359cd): new ApplicationsDatabaseBaseTests now verifies key validation paths in RegisterApplication, FindApplications, and QueryApplications, including success and invalid-argument scenarios.

@romanett
Copy link
Copy Markdown
Contributor Author

romanett commented May 13, 2026

@copilot please add the FindMethodState / FindMethodStateAsync to INodeManger3/IAsyncNodeManager interfaces directly. Also use the Methods in the CustomNodeManager / AsyncCustomNodeManager in the Call / CallAsync paths to reduce code duplication. In the MasterNodeManager use IAsyncNodeManager directly and rely on the adapter to resolve the method. Also fix test failures.

Copy link
Copy Markdown
Contributor

Copilot AI commented May 13, 2026

@copilot please add the FindMethodState / FindMethodStateAsync to INodeManger3/IAsyncNodeManager interfaces directly. Also use the Methods in the CustomNodeManager / AsyncCustomNodeManager in the Call...

Done in bb34a01 (refined in 016acf2): FindMethodState/FindMethodStateAsync are now on INodeManager3/IAsyncNodeManager directly, CustomNodeManager + AsyncCustomNodeManager call paths now use those methods to avoid duplicated method-resolution logic, and MasterNodeManager now resolves via IAsyncNodeManager directly (adapter-backed). I also fixed the related test failures and re-ran targeted server/GDS tests successfully.

@romanett
Copy link
Copy Markdown
Contributor Author

romanett commented May 14, 2026

@copilot please fix the failing GDS tests

Copy link
Copy Markdown
Contributor

Copilot AI commented May 14, 2026

@copilot please fix the failing GDS tests

Fixed in 636fd84. I updated the failing GDS tests in ClientTest to match current server behavior (BadEntryExists on duplicate registration, BadWriteNotSupported on ApplicationUri updates, and a robust non-empty assertion for QueryAllApplicationsAsync). I also reran the full GDS Release+coverage test command used in CI successfully.

@romanett
Copy link
Copy Markdown
Contributor Author

romanett commented May 14, 2026

@copilot please fix the aot gds TestsFixture to Work with an RSA Security policy

Copy link
Copy Markdown
Contributor

Copilot AI commented May 14, 2026

@copilot please fix the aot gds TestsFixture to Work with an RSA Security policy

Fixed in 9f7d8d2. I updated the AOT GDS fixture to provision/validate client certificates before connect, prefer RSA SignAndEncrypt endpoint selection, and constrain the test server to an RSA application certificate. I also updated the AOT update test to align with current GDS update semantics and reran the AOT publish+native test run successfully (71/71 passed).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@marcschier marcschier marcschier approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@romanett @CLAassistant @marcschier